Skip to content

Fixed use of undefined 'types' array in BinaryenAddGlobal tracing#1279

Merged
kripken merged 2 commits intoWebAssembly:masterfrom
dcodeIO:c-addglobal-fix
Nov 10, 2017
Merged

Fixed use of undefined 'types' array in BinaryenAddGlobal tracing#1279
kripken merged 2 commits intoWebAssembly:masterfrom
dcodeIO:c-addglobal-fix

Conversation

@dcodeIO
Copy link
Copy Markdown
Contributor

@dcodeIO dcodeIO commented Nov 9, 2017

The tracing code for BinaryenAddGlobal references a types array that doesn't seem to exist. All other functions use just type directly.

Appears that BinaryenAddGlobal isn't part of any (tracing) test, btw.

@dcodeIO
Copy link
Copy Markdown
Contributor Author

dcodeIO commented Nov 10, 2017

Not sure why the build shows as errored (besides that I didn't add a test because I don't remember how to re-generate the .txt fixtures). The different steps completed successfully.

@kripken
Copy link
Copy Markdown
Member

kripken commented Nov 10, 2017

Build looks ok, merging.

To recreate the .txt files, you can run ./auto_update_tests.py

@kripken kripken merged commit 5b44a8b into WebAssembly:master Nov 10, 2017
@dcodeIO
Copy link
Copy Markdown
Contributor Author

dcodeIO commented Nov 11, 2017

Currently trying to regenerate the tests. On Windows, this results in .wast files using 0xD 0xD 0xA line endings (literally double 0xD), and it eventually terminates with

[parse exception: inline string contains NULL (0). that is technically valid in wasm, but you shouldn't do it, and it's not supported in binaryen]Fatal: error in parsing input
Traceback (most recent call last):
  File "auto_update_tests.py", line 103, in <module>
    actual += run_command(cmd)
  File "...\binaryen\scripts\test\support.py", line 161, in run_command
    raise Exception(('run_command failed (%s)' % code, out + str(err or '')))
Exception: ('run_command failed (1)', '')

which is thrown in wasm-binary.cpp:1399 after evaluating the characters (then followed by \0)

f
a
c
-
o
p
t
�
ù
Ç
Ç
Ç

Not sure if that helps in any way, but I wasn't able to figure this out yet.

@kripken
Copy link
Copy Markdown
Member

kripken commented Nov 13, 2017

Very odd. The auto update script is pretty simple, just runs the commands and saves the output, so not sure how something could go wrong.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants